Repository navigation
docs: add ADR-0021 remote operations API - #124
Merged
Merged
Conversation
Restore the missing parent/child eval golden fixture so integration e2e stays green, and define HTTP-first remote operations as the P1 boundary.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds golden evaluation test files for parent-child subagent scenarios and introduces ADR-0021, which specifies a P1 HTTP-first Remote Session and Workspace Operations API with token-scoped authorization, manifest-first archive download safety, and CLI as a thin HTTP client for remote operations. ChangesParent-Child Subagent Evaluation Tests
Remote Session and Workspace Operations API (ADR-0021)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
coding-agent/docs/adr/0021-remote-session-and-workspace-operations-api.md (4)
201-204: ⚡ Quick winSpecify stable owner label derivation more precisely.
The guidance to "derive a stable owner label from the authenticated token, such as a hash or configured token label" lacks implementation details. Consider specifying:
- If using a hash: which algorithm (SHA-256?) and what input (full token? prefix?)
- Collision handling strategy
- Or recommend token-to-label mapping via configuration
Vague authorization logic can lead to inconsistent or weak implementations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/docs/adr/0021-remote-session-and-workspace-operations-api.md` around lines 201 - 204, Specify deterministic label derivation: compute a SHA-256 hex of the full authenticated token and prefix it (e.g. "owner:") to form the stable owner label for P1; on creation, check a persistent TokenLabelMap (config-backed or DB) to detect collisions and if a collision occurs, disambiguate by appending a short counter or using an HMAC with a per-deployment salt stored in configuration; additionally allow an explicit token-to-label mapping via configuration (TokenLabelMap) to override the hash-derived label and document that the system must persist and consult this mapping whenever resolving owner labels.
45-66: ⚡ Quick winClarify which endpoints are new vs. existing.
The "canonical P1 session API" list (lines 45-52) and "Existing endpoints remain compatible" list (lines 54-62) have significant overlap (
POST /prompt,GET /events,GET /DELETEsessions endpoints appear in both), making it unclear which endpoints P1 is adding versus preserving.Consider restructuring to explicitly call out:
- New in P1:
GET /sessions,POST /cancel, workspace archive manifest endpoints- Existing (preserved): All others
This would help implementers understand the delta more clearly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/docs/adr/0021-remote-session-and-workspace-operations-api.md` around lines 45 - 66, The two endpoint lists are ambiguous because several routes (e.g., POST /sessions/{session_id}/prompt, GET /sessions/{session_id}/events, GET /sessions/{session_id}, DELETE /sessions/{session_id}) appear in both; update the document to clearly separate "New in P1" and "Existing (preserved)" sections: under "New in P1" list only GET /sessions, POST /sessions/{session_id}/cancel, GET /sessions/{session_id}/workspace/archive/manifest, and GET /sessions/{session_id}/workspace/archive; under "Existing (preserved)" list POST /sessions, POST /sessions/{session_id}/prompt, POST /sessions/{session_id}/approve, GET /sessions/{session_id}/events, GET /sessions/{session_id}, DELETE /sessions/{session_id}, and GET /sessions/{session_id}/workspace (with a note that GET /sessions/{session_id}/workspace is a compatibility alias for /workspace/archive and new clients should use /workspace/archive/manifest then /workspace/archive).
150-150: ⚡ Quick winClarify "cheap to compute" criteria for SHA-256.
The phrase "when cheap to compute without double-exporting" is subjective. Consider specifying concrete criteria (e.g., "when the archive is already materialized" or "when total_bytes < 100MB") to ensure consistent manifest behavior across implementations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/docs/adr/0021-remote-session-and-workspace-operations-api.md` at line 150, Replace the vague clause for the manifest field `archive_sha256` ("when cheap to compute without double-exporting") with concrete, testable criteria—for example: "include `archive_sha256` when the archive is already materialized (e.g., stored on disk or in object storage) or when the archive size is below a defined threshold (e.g., total_bytes < 100MB)." Update the wording in the ADR section that currently reads "when cheap to compute without double-exporting" to one or two specific rules (materialized state and/or byte-size threshold) and add a short note that implementations must document which rule they follow.
364-364: 💤 Low valueRemove trailing line number artifact.
Line 364 appears to contain only a line number without content, likely a formatting artifact from the annotation process.
🧹 Proposed fix
- `tests/coding_agent/environment/test_docker_workspace_provider.py` -365🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/docs/adr/0021-remote-session-and-workspace-operations-api.md` at line 364, Remove the stray trailing artifact line that contains the lone string "tests/coding_agent/environment/test_docker_workspace_provider.py" from the ADR document (0021-remote-session-and-workspace-operations-api.md); locate the isolated line 364 with that content and delete it so the file no longer includes the extraneous line number/artifact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@coding-agent/docs/adr/0021-remote-session-and-workspace-operations-api.md`:
- Around line 201-204: Specify deterministic label derivation: compute a SHA-256
hex of the full authenticated token and prefix it (e.g. "owner:") to form the
stable owner label for P1; on creation, check a persistent TokenLabelMap
(config-backed or DB) to detect collisions and if a collision occurs,
disambiguate by appending a short counter or using an HMAC with a per-deployment
salt stored in configuration; additionally allow an explicit token-to-label
mapping via configuration (TokenLabelMap) to override the hash-derived label and
document that the system must persist and consult this mapping whenever
resolving owner labels.
- Around line 45-66: The two endpoint lists are ambiguous because several routes
(e.g., POST /sessions/{session_id}/prompt, GET /sessions/{session_id}/events,
GET /sessions/{session_id}, DELETE /sessions/{session_id}) appear in both;
update the document to clearly separate "New in P1" and "Existing (preserved)"
sections: under "New in P1" list only GET /sessions, POST
/sessions/{session_id}/cancel, GET
/sessions/{session_id}/workspace/archive/manifest, and GET
/sessions/{session_id}/workspace/archive; under "Existing (preserved)" list POST
/sessions, POST /sessions/{session_id}/prompt, POST
/sessions/{session_id}/approve, GET /sessions/{session_id}/events, GET
/sessions/{session_id}, DELETE /sessions/{session_id}, and GET
/sessions/{session_id}/workspace (with a note that GET
/sessions/{session_id}/workspace is a compatibility alias for /workspace/archive
and new clients should use /workspace/archive/manifest then /workspace/archive).
- Line 150: Replace the vague clause for the manifest field `archive_sha256`
("when cheap to compute without double-exporting") with concrete, testable
criteria—for example: "include `archive_sha256` when the archive is already
materialized (e.g., stored on disk or in object storage) or when the archive
size is below a defined threshold (e.g., total_bytes < 100MB)." Update the
wording in the ADR section that currently reads "when cheap to compute without
double-exporting" to one or two specific rules (materialized state and/or
byte-size threshold) and add a short note that implementations must document
which rule they follow.
- Line 364: Remove the stray trailing artifact line that contains the lone
string "tests/coding_agent/environment/test_docker_workspace_provider.py" from
the ADR document (0021-remote-session-and-workspace-operations-api.md); locate
the isolated line 364 with that content and delete it so the file no longer
includes the extraneous line number/artifact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9607849-ae77-4a07-bf5f-53002c32c32b
📒 Files selected for processing (3)
coding-agent/data/eval/golden/parent-child-subagent-001.jsonlcoding-agent/data/eval/golden/parent-child-subagent-001.yamlcoding-agent/docs/adr/0021-remote-session-and-workspace-operations-api.md
Clarify new versus preserved session endpoints, make archive checksum rules testable, define deterministic token owner labels, and remove the extra trailing reference.
This was referenced May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
Release Notes